@mitchmindtree would love some review on this when you have a moment. refactored the 32 bit implementation per @kawogi 's great notes. I also added a first pass at the 24 bit implementation.

I know the conversions are working correctly because I was able to play back 24 bit and 32 bit audio on macos (after converting to f32). PR checks are breaking because I have to resolve 24 bit for hosts that I know support 24 bit audio but I'm unable to verify how I should implement it (yet) - ALSA, ASIO, WASAPI. I wanted to get your input on the API and what I've done so far before proceeding further.

in the original issue I opened #414, we discussed how to structure the API for 24 bit audio. one idea was to store 24 bit as packed data, but I ended up going the opposite route with this and storing the 24 bit unpacked in an i32. I did this to make it more ergonomic in working with other numbers and converting to other formats - since 24 bit audio appears to be the least supported output, it's more likely it'll be scaled up / down.

If you want, I can change this to use the packed 3 bytes, but it still would require converting to i32 for doing math.

I also couldn't find any examples of how ALSA, ASIO, or WASAPI handle 24 bit audio - do you know if they just look for 3 byte buffers or unpacked 32 bit numbers in their native endianness? I need to test myself I guess, but I still need to set up my local dev env for those targets so insight would be helpful if you happen to have any.

thanks!

built a new PC recently so going to be testing my branch of cpal via my music player on linux and PC regularly soon, hopefully will gain some insight into how 24 bits can bee stored ergonomically to work best across platform. i'll also investigate fixing the tests for those platforms

in the meantime, still would love any feedback if you have any, anything you're looking for, or if I should stop asking about this MR, etc. thanks!

Sorry about the wait @julientregoat, and thanks for your work on this!

I think the direction you're going in makes sense. The i32 support seems pretty straight forward, but yes the I24 stuff can get ambiguous quickly. W.r.t. packed or unpacked data - in the end we'll probably end up wanting to support both cases, as there will undoubtedly be some audio APIs that deliver packed and some that deliver unpacked, and if we only offer one or the other, we would need to do some conversion to the one that we support. That said, I'm not yet sure how we should go about exposing packed data in the callback yet. W.r.t. individual I24 samples, I think your current approach of having them unpacked in an i32 makes sense for all the reasons you mention.

Also I'm not sure if we've already discussed this together, but there has been some discussion in the past about potentially using the dasp_sample crate to remove a little responsibility from cpal and add a little more general flexibility. It declares quite a few types (including unpacked I24 and I48) sample types, but I haven't given it a great deal of thought yet. It very well might make more sense to just stick to CPAL maintaining its own trait, and it seems we'd still need the SampleFormat enum anyway. Just thought I'd mention it in case you were interested in looking into this as an option.

No worries, appreciate you taking the time to look over this!

OK cool re: packed vs unpacked. I definitely agree that both will be needed eventually, I think for now it makes sense to hold off on deciding a packed interface until we cross that bridge so we can keep all options open. The to_be_bytes / to_le_bytes also give us access to the sample in those forms as well and may suffice in the meantime.

Hadn't seen those dasp crates yet, that's handy. The format support is pretty awesome, and seems like it would be useful for future expansion as well. also especially wrt upscaling / downscaling conversions - it definitely makes sense to offload that responsibility, especially since the code has already been written in dasp. I also appreciate that it aims to standardize fundamental traits that the rest of the community can build on. cpal using it would set a good example & encourage usage. Plus, no dependencies makes it an easier decision, and I think it would remove a lot of code in sample_formats.rs.

I believe we'll need our own trait either way so we can map types to SampleFormats, which could be a minimized version of the existing trait, maybe something like the following:

trait Sample: dasp_sample::Sample {
    const FORMAT: SampleFormat;
}

I'm hesitant to say we should do that with this PR but don't have any good reasons. Something about not having direct control of the structs or traits? Like you mentioned, we'll probably need a few different i24 interfaces but I guess we could implement the dasp_sample::Sample trait for packed versions. I'm not sure, maybe just because it's a significant change and I'm not confident how it would affect cpal users; probably just need to think about it more.

I'm going to get this PR working with linux targets next and hopefully get a chance to play with the 24 + 32 bit ALSA targets if I have some devices that register with one of the following formats

            SND_PCM_FORMAT_S24_LE,
            SND_PCM_FORMAT_S24_BE,
            SND_PCM_FORMAT_S32_LE,
            SND_PCM_FORMAT_S32_BE,
            SND_PCM_FORMAT_S24_3LE,
            SND_PCM_FORMAT_S24_3BE,

I'm looking forward to having this merged!
Just one comment: I get the impression that i32 is done, while i24 needs more work. If that is correct, then I think splitting the PR and merging just the i32 part asap would be a good idea.

@HEnquist yeah that’s a fair point, I’ll split it out tonight and open a separate PR

alright @mitchmindtree I've extracted the 24 bit stuff into a separate PR and finished off any lingering issues in this one. I've tested that it converts to the other sample formats successfully, and tested it with a native 32 bit ALSA output. I haven't been able to test it with a native 32 bit ASIO output yet

What's missing to this?

What's missing to this?

It needs a rebase at the least. Then a review. Refactoring to use dasp_sample might be an idea but is a longer term thing.

@lu-zero @est31 I also still haven't been able to test directly with 32 bit output on ASIO but did check input. This may need to be tested again though since I had to patch the input conversion just now thanks to @lu-zero's comment